Skip to content

Force id to be mandatory #284

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

fjtirado
Copy link
Collaborator

@fjtirado fjtirado commented Oct 27, 2023

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:
This makes id mandatory, keeping current constructor unchanged from the previous version

Special notes for reviewers:

Additional information (if needed):
SDK has currently two misaligments with the spec: id and name are mandatory, while the former is mandatory only if there is no key and the latter is optional. Since making id optionally depending on key is not a good idea that will be probably changed in 0.9 (where ID will be mandatory always and key removed or make optional), I think is better to postpone the API breaking change to the migration to 0.9

Signed-off-by: Francisco Javier Tirado Sarti <ftirados@redhat.com>
@fjtirado fjtirado force-pushed the id_really_mandatory branch from 01a4c01 to 44d6996 Compare October 27, 2023 15:50
@ricardozanini
Copy link
Member

ricardozanini commented Oct 27, 2023

This PR partially reverts what was done in this one:

8fbc3bf

The author in that commit, by adding key, enforced the spec. This is fine, but broke the API contract since the json schema generator is now different by not having the id as a parameter.

Originally, the SDK forced the id to be required. We can review this in another major version/step. Also see: serverlessworkflow/specification#742

@ricardozanini
Copy link
Member

@manick02 can you take a look too please?

Copy link
Contributor

@manick02 manick02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@ricardozanini ricardozanini merged commit 2505fc3 into serverlessworkflow:main Nov 1, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants